Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactored Factory::getUser() to ::getApplication()->getIdentity() #37321

Closed
wants to merge 2 commits into from

Conversation

pe7er
Copy link
Contributor

@pe7er pe7er commented Mar 19, 2022

Joomla 4.1 uses the deprecated Factory::getUser(). I've changed it in a couple of files to Factory::getApplication()->getIdentity().

  • Does it make sense to manually replace it like this?
  • Or will it be done automatically with a global search & replace?
  • Please advice me if I could continue to refactor it like this in other files as well?

Summary of Changes

This PR changes Factory::getUser in 5 files to Factory::getApplication()->getIdentity.
In some cases I added Exception to the code block and as name space.

$user = Factory::getUser($userId);
to
$user = Factory::getApplication()->getIdentity($userId);

$user = Factory::getUser();
to
$user = Factory::getApplication()->getIdentity();

if (!Factory::getUser()->authorise('core.admin'))
to
if (!Factory::getApplication()->getIdentity()->authorise('core.admin'))

Testing Instructions

Please do a code inspection.
It should not change any functionality and Joomla should continue work correctly.

Comment on lines +54 to +57
* @param array $messages The contents of the messages to be logged
* @param string $messageLanguageKey The language key of the message
* @param string $context The context of the content passed to the plugin
* @param int $userId ID of user perform the action, usually ID of current logged in user
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert.

@laoneo
Copy link
Member

laoneo commented Mar 20, 2022

Thanks for your pr. Honestly I would like to see a change to introduce a user service which can be injected into the view, to get a user from rather exchanging a static function call with another.

@sandewt
Copy link
Contributor

sandewt commented Mar 20, 2022

A small addition:

Factory::getUser()->id
or
Factory::getUser()->get('id')
to
Factory::getApplication()->getIdentity()->id

@sandewt
Copy link
Contributor

sandewt commented Mar 20, 2022

It should not change any functionality and Joomla should continue work correctly.

Point of interest:
#30834 (comment) and
#30834 (comment)

@wilsonge
Copy link
Contributor

Generally speaking this is OK. Worth noting that behaviours aren't exactly identical as noted in #36448 so you can't do a full auto-replace in an IDE.

@laoneo HTMLView's at least will also be hard coupled to the application for the active template (https://github.com/joomla/joomla-cms/blob/4.1-dev/libraries/src/MVC/View/HtmlView.php#L377). So unless we plan to start injecting that whilst obviously the statics aren't ideal - it might be that we inject the application into the view in the constructor going forwards anyhow. Even if we inject the user because there's going to be authentication checks throughout the view probably makes more sense in the constructor than as a service there anyhow.

@laoneo
Copy link
Member

laoneo commented Mar 20, 2022

What about the UserFactory?

@joomdonation
Copy link
Contributor

@pe7er You might want to close this PR because we now use a different approach:

@@ -44,7 +44,7 @@ class ActionlogModel extends BaseDatabaseModel
*/
public function addLog($messages, $messageLanguageKey, $context, $userId = null)
{
$user = Factory::getUser($userId);
$user = Factory::getApplication()->getIdentity($userId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for information the change here is not right. The getIdentity method does not accept any parameter like you passed here. If we want to replace it, we will need to use UserFactory, something like:

$user = Factory::getContainer()->get(UserFactoryInterface::class)->loadUserById($userId);

@laoneo
Copy link
Member

laoneo commented Jun 27, 2022

Agree, this one should be closed as users will be injected into views and models.

@HLeithner
Copy link
Member

This pull request has automatically rebased to 4.2-dev.

@HLeithner HLeithner changed the base branch from 4.1-dev to 4.2-dev June 27, 2022 13:05
@pe7er
Copy link
Contributor Author

pe7er commented Jun 29, 2022

@joomdonation thanks for your explanation and for your PR to solve this in a better way
@laoneo thanks for your PR to solve this in a better way

closing this one...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants